Add chatIdOrNull to duckchat api module and drop chatID string duplication#8631
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
GerardPaligot
force-pushed
the
feature/gerard/duckchat-extract-chatid
branch
from
09514e4 to
48d4109
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 48d4109. Configure here.
GerardPaligot
changed the title
The "chatID" query parameter name was duplicated across three call sites extracting the chatID from a Duck.ai URL, each implementing the lookup differently and only two of them gating on isDuckChatUrl. Lift the operation onto the public DuckChat interface as chatIdOrNull(uri) so the literal lives in one place and the safety guard is enforced for every consumer — closing a latent foot-gun in DuckChatContextualViewModel.hasChatId that previously trusted any URL carrying a chatID query param. DuckChatConstants.CHAT_ID_PARAM is removed; the raw "chatID" string now only lives in RealDuckChat's private const. URL building stays on the impl-only DuckChatInternal since every builder is impl-internal.
…lication The "chatID" query parameter name was extracted in three different shapes across modules — a custom Uri extension in :app, a private helper in :duckchat-impl, and an inline lookup in DuckChatContextualViewModel — only two of which gated on isDuckChatUrl. Lift the operation onto a top-level Uri.toChatIdOrNull(DuckChat) extension in :duckchat-api so the literal lives in one place and the safety guard is enforced for every consumer, closing a latent foot-gun in DuckChatContextualViewModel.hasChatId that previously trusted any URL carrying a chatID query param. URL building stays on the impl-only DuckChatInternal since every builder is impl-internal and never crosses the module boundary.
buildChatUrl was the only Duck.ai URL builder that did not call
nativeInputParameters(), so callers that previously composed their URL
via getDuckChatUrl("", false) + manual appendQueryParameter("chatID", …)
silently lost the native-input=true param after migrating onto
buildChatUrl(chatId). Pull nativeInputParameters() into buildChatUrl so
the "single source of truth for the Duck.ai chat URL shape" actually
produces the same URL shape as every other builder.
Adds two RealDuckChatTest cases that pin the contract: with the native
input setting on, buildChatUrl emits ?chatID=…&native-input=true&duckai=5;
with it off, native-input is absent.
GerardPaligot
force-pushed
the
feature/gerard/duckchat-extract-chatid
branch
from
5a6e8c1 to
825f604
Compare
malmstein
left a comment
malmstein
left a comment
There was a problem hiding this comment.
did not install or smoke-test the APK. nice cleanup — folding the four chatID extractors into one toChatIdOrNull is the right move, and the learnings log from #8666 explicitly flagged the multi-extractor sprawl as a coordination smell, so it's good to see it resolved. centralising chat-url construction through buildChatUrl is also the right direction. just one minor question on the stricter isDuckChatUrl gating in DuckChatContextualViewModel.extractChatId, nothing blocking.
| private fun extractChatId(url: String?): String? = | ||
| url?.toUri()?.getQueryParameter(CHAT_ID_PARAM)?.takeIf { it.isNotBlank() } | ||
| private fun extractChatId(url: String?): String? { | ||
| val uri = url?.toUri() ?: return null |
There was a problem hiding this comment.
nit: this gates on isDuckChatUrl via toChatIdOrNull where it didn't before — the test fake had to be relaxed to recognise duck.ai/duckduckgo.com hosts. probably fine since the sheet always loads duck.ai urls by construction, but worth flagging in case there's an edge case (redirects, malformed urls) where we'd want the old leniency.
There was a problem hiding this comment.
Yes, that's true, but even though there is no check before in this function, the URL is initialized by the DuckChat.getDuckChatUrl function, so we won't break anything. We are just being more restrictive.
3 participants
Task/Issue URL: https://app.asana.com/1/137249556945/task/1214962354449266
Description
Promotes the
"chatID"query parameter handling out of three impl-private helpers into one function on the publicDuckChatinterface:chatIdOrNull(uri: Uri): String?. As a side effect,DuckChatContextualViewModel.hasChatIdnow correctly gates onisDuckChatUrl— previously a crafted URL likehttps://evil.com?chatID=abcwas treated as a Duck.ai chat.Steps to test this PR
Chat-history clears (DuckChatDataClearingPlugin + DuckAiTabsCleanupPlugin)
Chat resume from input suggestions
chatIDin the URL.UI changes
n/a
Note
Medium Risk
Changes affect chat deletion/tab matching and contextual fullscreen behavior; the stricter chatID gating fixes mis-detection on non-Duck.ai URLs but alters edge-case URL handling.
Overview
Introduces
Uri.toChatIdOrNull(duckChat)induckchat-apiand replaces several impl-localchatIDparsers (native input, data clearing, tab cleanup, contextual sheet) with that helper, removing the sharedCHAT_ID_PARAMconstant.DuckChatContextualViewModelnow treats a URL as having a chat only when it passesisDuckChatUrl, not merely when achatIDquery param is present on any host.buildChatUrlis the single place for resume links: it addsnative-inputwhen that setting is on, and the input screen / native input widget call it instead of hand-building URIs. Tests coverbuildChatUrlwith native input on/off.Reviewed by Cursor Bugbot for commit 825f604. Bugbot is set up for automated code reviews on this repo. Configure here.